Allow users to customize ofType columns during table creation #223#9580
Allow users to customize ofType columns during table creation #223#9580mzabuawala wants to merge 1 commit intopgadmin-org:masterfrom
Conversation
WalkthroughBackend now records original DEFAULT and NOT NULL for columns inherited from composite types; frontend tracks inheritance and permits editing OF TYPE columns; SQL templates render modified OF TYPE columns using WITH OPTIONS, while parent-table inheritance remains comment-only. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as Frontend (Column UI)
participant API as Backend API
participant Gen as SQL Template Generator
User->>UI: Edit or inspect column (may be OF TYPE)
UI->>UI: isInheritedFromType(), read hidden original_* fields
UI->>API: Request/submit column metadata/changes
API->>API: get_oftype() adds original_defval/original_attnotnull, marks inheritedfromtype
API->>Gen: Provide column metadata (including has_with_options/inheritedfromtable)
Gen->>Gen: Select rendering branch (WITH OPTIONS / inherited-from-table comment / regular)
Gen-->>API: Return generated CREATE SQL
API-->>UI: Send updated metadata and SQL
UI-->>User: Display SQL and editing outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mzabuawala
force-pushed
the
fixes_ofType_223
branch
2 times, most recently
from
c1e7467 to
d63a40f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js`:
- Around line 260-266: The map callback that builds inc_ids leaves undefined
entries when the if condition is false; update the logic around keys.map(...)
(the block assigning inc_ids) to only produce numeric ids for columns present in
currPk.include — for example replace the single map with a filter+map or a
reduce so you first filter keys where
currPk.include.indexOf(actionObj.oldState.columns[k].name) > -1 and then
parseInt(k), or keep the map but return null/undefined and immediately follow
with .filter(Boolean) to remove empties; ensure the final inc_ids contains only
parsed integers.
- Around line 244-247: The map callback for keys.map only returns when the if
condition is true, producing undefined entries; change the logic to produce a
compact array of ints by using filter+map or flatMap: e.g., filter keys where
columns.indexOf(actionObj.oldState.columns[k].name) > -1 then map to
parseInt(k), or use keys.flatMap(k =>
columns.indexOf(actionObj.oldState.columns[k].name) > -1 ? [parseInt(k)] : []).
Update the expression that assigns ids (the keys.map usage and variable ids)
accordingly so every execution path returns a defined value.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/16_plus/create.sql`:
- Line 57: The template can produce invalid SQL if a column is marked WITH
OPTIONS while also being an identity column, so update the backend logic that
computes has_with_options to explicitly skip setting it for identity
constraints: when determining has_with_options for OF TYPE columns (the code
path that currently sets has_with_options based on DEFAULT, NOT NULL, or PRIMARY
KEY changes), add a guard that if colconstype == 'i' (identity) do not set
has_with_options; ensure the same check is applied wherever has_with_options is
derived or mutated so templates using has_with_options can never see true for
identity columns.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/default/create.sql`:
- Line 49: The template has a malformed conditional around rendering the column
type: wrap the direct type rendering ({{c.displaytypname}}) with an {% if is_sql
%} ... {% else %} ... {% endif %} so the existing {% else %} and {% endif %}
pair up correctly; specifically, after rendering the column name
({{conn|qtIdent(c.name)}}) insert {% if is_sql %}{{c.displaytypname}}{% else %}
and keep the existing GET_TYPE.CREATE_TYPE_SQL(conn, c.cltype, c.attlen,
c.attprecision, c.hasSqrBracket) branch as the else, ensuring the final {% endif
%} closes this new conditional.
🧹 Nitpick comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py (1)
367-375: Unusedpk_columnsset is built but never referenced.The
pk_columnsset is populated fromdata['primary_key'], but it's never used in the subsequent logic. The only reference is in a commented-out block (lines 389-391). Consider either:
- Removing this code if it's not needed
- Uncommenting the usage if primary key detection should trigger
has_with_options♻️ Option 1: Remove unused code
- # Get list of columns in primary key constraint - pk_columns = set() - if 'primary_key' in data and len(data['primary_key']) > 0: - for pk in data['primary_key']: - if 'columns' in pk: - for col in pk['columns']: - if 'column' in col: - pk_columns.add(col['column']) - for c in columns:
| ids = keys.map((k) => { | ||
| if (columns.indexOf(actionObj.oldState.columns[k].name) > -1) | ||
| return parseInt(k); | ||
| }); |
There was a problem hiding this comment.
Map callback should return a value on all execution paths.
This map() callback only returns a value when the condition is true, resulting in undefined values in the array when the condition is false. This is flagged by static analysis as it's semantically incorrect for map().
Consider using filter() with map() or flatMap() for clearer intent:
🔧 Proposed fix
- ids = keys.map((k) => {
- if (columns.indexOf(actionObj.oldState.columns[k].name) > -1)
- return parseInt(k);
- });
+ ids = keys
+ .filter((k) => columns.indexOf(actionObj.oldState.columns[k].name) > -1)
+ .map((k) => parseInt(k));🧰 Tools
🪛 Biome (2.3.13)
[error] 244-244: This callback passed to map() iterable method should always return a value.
Add missing return statements so that this callback returns a value on all execution paths.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js`
around lines 244 - 247, The map callback for keys.map only returns when the if
condition is true, producing undefined entries; change the logic to produce a
compact array of ints by using filter+map or flatMap: e.g., filter keys where
columns.indexOf(actionObj.oldState.columns[k].name) > -1 then map to
parseInt(k), or use keys.flatMap(k =>
columns.indexOf(actionObj.oldState.columns[k].name) > -1 ? [parseInt(k)] : []).
Update the expression that assigns ids (the keys.map usage and variable ids)
accordingly so every execution path returns a defined value.
| let inc_ids = keys.map((k) => { | ||
| if ( | ||
| currPk.include.indexOf(actionObj.oldState.columns[k].name) > | ||
| -1 | ||
| ) | ||
| return parseInt(k); | ||
| }); |
There was a problem hiding this comment.
Map callback should return a value on all execution paths.
Same issue as above - this map() callback doesn't return a value when the condition is false, producing undefined entries in the resulting array.
🔧 Proposed fix
- let inc_ids = keys.map((k) => {
- if (
- currPk.include.indexOf(actionObj.oldState.columns[k].name) >
- -1
- )
- return parseInt(k);
- });
+ let inc_ids = keys
+ .filter((k) => currPk.include.indexOf(actionObj.oldState.columns[k].name) > -1)
+ .map((k) => parseInt(k));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let inc_ids = keys.map((k) => { | |
| if ( | |
| currPk.include.indexOf(actionObj.oldState.columns[k].name) > | |
| -1 | |
| ) | |
| return parseInt(k); | |
| }); | |
| let inc_ids = keys | |
| .filter((k) => currPk.include.indexOf(actionObj.oldState.columns[k].name) > -1) | |
| .map((k) => parseInt(k)); |
🧰 Tools
🪛 Biome (2.3.13)
[error] 260-260: This callback passed to map() iterable method should always return a value.
Add missing return statements so that this callback returns a value on all execution paths.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/static/js/table.ui.js`
around lines 260 - 266, The map callback that builds inc_ids leaves undefined
entries when the if condition is false; update the logic around keys.map(...)
(the block assigning inc_ids) to only produce numeric ids for columns present in
currPk.include — for example replace the single map with a filter+map or a
reduce so you first filter keys where
currPk.include.indexOf(actionObj.oldState.columns[k].name) > -1 and then
parseInt(k), or keep the map but return null/undefined and immediately follow
with .filter(Boolean) to remove empties; ensure the final inc_ids contains only
parsed integers.
...owser/server_groups/servers/databases/schemas/tables/templates/tables/sql/16_plus/create.sql
Show resolved
Hide resolved
mzabuawala
force-pushed
the
fixes_ofType_223
branch
4 times, most recently
from
1aba96d to
4f03620
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js`:
- Around line 163-165: The edit/readonly logic currently treats any
inheritedfrom as table inheritance; update those gates to distinguish type
inheritance by using isInheritedFromType(state) so only true table inheritance
makes fields read-only. Specifically, replace direct checks of
state.inheritedfrom in the edit/readonly guards (and the
length/precision/default validation paths) with a check that inheritedfrom is
present AND isInheritedFromType(state) is false (e.g., only treat as
table-inherited when !isInheritedFromType(state) &&
!isEmptyString(state.inheritedfrom)). Also ensure get_oftype still preserves
compatibility but that any logic deciding editability consults
isInheritedFromType(state) to allow OF TYPE columns to be editable.
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py`:
- Around line 386-423: Remove the redundant is_primary_key-driven modification
flag: delete the check that sets has_modifications when c.get('is_primary_key')
is true so primary keys are not flagged as column-level changes; rely only on
actual DEFAULT/NOT NULL differences (original_defval/defval and
original_attnotnull/attnotnull) to set has_modifications, and keep the existing
identity check and has_with_options assignment for final_columns as-is (refer to
variables/keys c, is_primary_key, has_modifications, original_defval, defval,
original_attnotnull, attnotnull, has_with_options, final_columns).
...gadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js
Show resolved
Hide resolved
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py`:
- Around line 367-423: Remove the check that sets has_modifications when
c.get('is_primary_key') is true inside the columns loop so that PRIMARY KEY does
not cause has_with_options to be set; instead only detect modifications via
differences in defval (original_defval vs defval) and attnotnull
(original_attnotnull vs attnotnull) as currently implemented, and continue to
skip identity columns (colconstype == 'i') before marking c['has_with_options']
= True; locate this logic around the loop over columns and the variables
has_modifications, c.get('is_primary_key'), original_defval/current_defval, and
original_attnotnull/current_attnotnull.
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py
Show resolved
Hide resolved
|
Fixed all CodeRabbit review comments if possible. |
mzabuawala
force-pushed
the
fixes_ofType_223
branch
2 times, most recently
from
7e5ed2d to
56b65fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js (1)
372-378:⚠️ Potential issue | 🟠 MajorLength/precision editable checks still block OF TYPE columns.
The
attlenandattprecisioneditablefunctions use rawstate.inheritedfromchecks, which block editing for OF TYPE columns even though they should be editable. Thedefvalfield was already updated to useisInheritedFromType()to allow editing for type-inherited columns while blocking table-inherited ones.Update these checks to match the
defvalpattern:Suggested fix for attlen (lines 372-378)
editable: function(state) { - // inheritedfrom has value then we should disable it - if (!isEmptyString(state.inheritedfrom)) { + // Block editing for table inheritance, but allow for type inheritance + if (!isEmptyString(state.inheritedfrom) && !obj.isInheritedFromType(state)) { return false; } return Boolean(obj.attlenRange(state)); },Suggested fix for attprecision (lines 404-410)
editable: function(state) { - // inheritedfrom has value then we should disable it - if (!isEmptyString(state.inheritedfrom)) { + // Block editing for table inheritance, but allow for type inheritance + if (!isEmptyString(state.inheritedfrom) && !this.isInheritedFromType(state)) { return false; } return Boolean(this.attprecisionRange(state)); },
🤖 Fix all issues with AI agents
In
`@web/pgadmin/browser/server_groups/servers/databases/schemas/tables/templates/tables/sql/default/create.sql`:
- Line 49: The template has a mismatched Jinja2 conditional around rendering the
column type: after {{c.displaytypname}} there's an {% else %} with no matching
{% if %}, causing a syntax error; fix by replacing that fragment so the type
rendering is a proper if/else (i.e. test whether c.displaytypname is present and
render it, otherwise call GET_TYPE.CREATE_TYPE_SQL(conn, c.cltype, c.attlen,
c.attprecision, c.hasSqrBracket)), keeping the surrounding
inherited-from-type/parent-table branches intact and preserving references to
conn|qtIdent(c.name), c.inheritedfromtype, c.inheritedfromtable,
c.displaytypname and GET_TYPE.CREATE_TYPE_SQL.
🧹 Nitpick comments (2)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js (1)
493-518: Simplify the disabled logic for clarity.Lines 495-497 have unusual control flow - the
ifblock returnsisDisabled = falsebut immediately exits withreturn isDisabled = false, which is redundant and confusing.♻️ Suggested simplification
disabled: function(state) { let isDisabled = ['serial', 'bigserial', 'smallserial'].indexOf(state.cltype) > -1; - if(!isDisabled && this.isInheritedFromType(state)) { - return isDisabled = false; - } + // Allow editing for OF TYPE columns + if(this.isInheritedFromType(state)) { + return false; + } isDisabled = isDisabled || state.colconstype != 'n'; return isDisabled; },web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/utils.py (1)
367-375: Remove dead code or add TODO comment.The
pk_columnsset is built but never used (the usage is commented out on lines 390-391). This is dead code that adds unnecessary complexity.Either remove this block entirely, or if it's intentionally preserved for future use, add a clear TODO comment explaining the intent.
♻️ Option 1: Remove dead code
- # Get list of columns in primary key constraint - pk_columns = set() - if 'primary_key' in data and len(data['primary_key']) > 0: - for pk in data['primary_key']: - if 'columns' in pk: - for col in pk['columns']: - if 'column' in col: - pk_columns.add(col['column']) - for c in columns:
...owser/server_groups/servers/databases/schemas/tables/templates/tables/sql/default/create.sql
Outdated
Show resolved
Hide resolved
A composite type has its structure (data types, length, precision, scale, etc.) fully fixed at type definition time, so kept them as is. |
mzabuawala
force-pushed
the
fixes_ofType_223
branch
from
56b65fa to
6063ab9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/browser/server_groups/servers/databases/schemas/tables/columns/static/js/column.ui.js (1)
493-518:⚠️ Potential issue | 🟡 MinorKeep the
colconstypeguard even for OF TYPE columns.The early return when
isInheritedFromType()is true skips thecolconstype != 'n'check, so defaults can be edited for identity/generated columns, which can yield invalid SQL. Removing that early return preserves the intended OF TYPE enablement while keeping the identity/generated guard.🛠️ Suggested fix
disabled: function(state) { let isDisabled = ['serial', 'bigserial', 'smallserial'].indexOf(state.cltype) > -1; - if(!isDisabled && this.isInheritedFromType(state)) { - return isDisabled = false; - } isDisabled = isDisabled || state.colconstype != 'n'; return isDisabled; },
mzabuawala
force-pushed
the
fixes_ofType_223
branch
from
6063ab9 to
65c96db
Compare
2 participants
This pull request improves how tables that use the
OF TYPEclause (i.e., tables based on composite types) are handled in pgAdmin. The changes ensure that columns inherited from composite types are tracked for modifications, allowing the UI and generated SQL to correctly use theWITH OPTIONSsyntax when defaults or constraints are changed. This results in more accurate CREATE TABLE statements and better support for editing inherited columns.Enhancements for OF TYPE columns:
__init__.py, columns inherited from composite types now have aninheritedfromtypefield and store their original default and NOT NULL values, enabling the backend and UI to detect and compare modifications.columns/utils.py, the column parsing logic was updated to include inherited columns with modifications (such as changed defaults or NOT NULL constraints) in the final column list, marking them forWITH OPTIONSsyntax in the generated SQL.Improvements to CREATE TABLE SQL generation:
create.sqlfor versions 11+, 12+, 14+, 16+, and default), columns inherited from composite types that are modified now use theWITH OPTIONSsyntax, ensuring the generated SQL accurately reflects changes to defaults or constraints. [1] [2] [3] [4] [5]Summary by CodeRabbit
New Features
Bug Fixes
Improvements